stash: partial stash specific files by gitkraken-jacobw · Pull Request #10 · nodegit/libgit2

gitkraken-jacobw · 2022-06-29T17:18:57Z

Copy-pasted from main libgit2 repo:

There's this concept called partial stashing in Git where users are able to stash desired files without having to stash and also reset the entire workdir. I based my implementation on tiennou's suggestion from libgit2#4725. This introduces options to git_stash_save where developers are able to supply the paths that they want to have stashed from the workdir. I could have gone further and have exposed a git_index* option as tiennou suggested, but I chose not to at this moment. Let me know if I should expose a git_index* option so that users can supply an arbitrary index to be stashed.

I also introduced GIT_STASH_KEEP_ALL, which prevents resetting the index and workdir once a stash has been completed.

AlexaXs

The code looks very good, just maybe it would help with the review if you could add different commits, each one explaining the changes applied. I can think of these two:

a commit explaining the fact that code of git_stash_save() has been moved inside the new function git_stash_save_with_opts() and that this function handles two different cases, case when no paths are specified and case with paths for partial stashing.
a commit explaining the changes in commit_worktree()

AlexaXs · 2022-07-13T14:44:38Z

include/git2/stash.h

+ * or error code.
+ */
+GIT_EXTERN(int) git_stash_save_with_opts(
+	git_oid *out, git_repository *repo, git_stash_save_options *opts);


opts should be const: const git_stash_save_options *opts

we should keep the format here of other functions: \n and \t for each argument of the function, same way as for example git_stash_apply

AlexaXs · 2022-07-13T14:48:51Z

src/stash.c

+static int stash_update_index_from_paths(
+	git_repository *repo,
+	git_index *index,
+	git_strarray *paths)


const git_strarray *paths

AlexaXs · 2022-07-13T14:49:33Z

src/stash.c

 	git_commit *b_commit,
-	git_commit *u_commit)
+	git_commit *u_commit,
+	git_tree *tree)


const git_tree *tree

AlexaXs · 2022-07-13T14:50:09Z

src/stash.c

+static int ensure_there_are_changes_to_stash_paths(
+	git_repository *repo,
+	uint32_t flags,
+	git_strarray *paths)


const git_strarray *paths

AlexaXs · 2022-07-13T14:50:41Z

src/stash.c

+}
+
+int git_stash_save_with_opts(
+	git_oid *out, git_repository *repo, git_stash_save_options *opts)


const git_stash_save_options *opts

AlexaXs · 2022-07-13T15:02:26Z

src/stash.c

 	GIT_ASSERT_ARG(out);
 	GIT_ASSERT_ARG(repo);
-	GIT_ASSERT_ARG(stasher);



We also need to check if opts is NULL, and if so provide a default one. Similar to what normalize_apply_options does at the beginning.

Since it appears opts.stasher is required, should we instead do something like:

GIT_ASSERT_ARG(opts); GIT_ASSERT_ARG(opts->stasher); // not sure if this is the best way to validate properties

It seems GIT_ASSERT_ARG is used when NULL is not allowed for the argument, and checking for NULL and providing a default value is used when NULL is allowed. So I agree we can use GIT_ASSERT_ARG(opts) inside git_stash_save_with_opts.

The problem is when the user calls git_stash_save, GIT_STASH_SAVE_OPTIONS_INIT initializes stasher with NULL, in which case if we don't allow stasher to be NULL inside git_stash_save_with_opts it won't work.

I think a possible solution could be GIT_ASSERT_ARG(stasher) inside git_stash_save, and GIT_ASSERT_ARG(opts && opts->stasher) inside `git_stash_save_with_opts

AlexaXs · 2022-07-13T15:05:20Z

include/git2/stash.h

+ * @param version The struct version; pass `GIT_STASH_SAVE_OPTIONS_VERSION`.
+ * @return Zero on success; -1 on failure.
+ */
+GIT_EXTERN(int) git_stash_save_options_init(


would be good to add a test in tests/core/structinit.c similar to this one:

/* stash apply */ CHECK_MACRO_FUNC_INIT_EQUAL( \ git_stash_apply_options, GIT_STASH_APPLY_OPTIONS_VERSION, \ GIT_STASH_APPLY_OPTIONS_INIT, git_stash_apply_options_init);

AlexaXs · 2022-07-13T15:09:43Z

src/stash.c

 		goto cleanup;

-	if ((error = ensure_there_are_changes_to_stash(repo, flags)) < 0)
+	if (opts->paths.count == 0 &&


Probably it could clear the code a bit if we added a flag like bool hasPaths assigning it the value of opts->paths.count > 0, and use that flag when testing this condition in the code: if (!hasPaths)...

AlexaXs · 2022-07-13T15:10:43Z

src/stash.c

-	if ((error = commit_worktree(out, repo, stasher, git_buf_cstr(&msg),
-				     i_commit, b_commit, u_commit)) < 0)
-		goto cleanup;
+	if (opts->paths.count == 0) {


if (!hasPaths)...

AlexaXs · 2022-07-13T15:22:02Z

src/stash.c

 	git_commit_free(i_commit);
 	git_commit_free(b_commit);
 	git_commit_free(u_commit);
+	git_tree_free(tree);


_free() functions are safe with NULL, but since we are mixing code here for cases with paths and without paths, it'd be good to do this here too:

if (hasPaths) { git_tree_free(tree); git_reference_free(head); git_index_free(paths_index); }

we want to build a partial stash from an index in the future, so some functions have been reworked to allow for this

code has been moved from `git_stash_save` into `git_stash_save_with_opts` in order to handle cases where we want to partially stash using options or stash normally without partially stashed paths specified

AlexaXs suggested changes Jul 13, 2022

View reviewed changes

gitkraken-jacobw added 6 commits July 14, 2022 10:18

stash: introduce build_stash_commit_from_index

d6554d0

we want to build a partial stash from an index in the future, so some functions have been reworked to allow for this

stash: implement partial stashing by path

8280bb0

code has been moved from `git_stash_save` into `git_stash_save_with_opts` in order to handle cases where we want to partially stash using options or stash normally without partially stashed paths specified

stash: implement CI testing

65210e9

stash: better option validation for stash save

ec67f95

stash: add const to arguments

f2befe8

stash: test save options init

98faa3e

gitkraken-jacobw force-pushed the partialstashing branch from 7d7c7f2 to 98faa3e Compare July 14, 2022 17:19

gitkraken-jacobw mentioned this pull request Jul 27, 2022

stash: partial stash specific files Axosoft/libgit2#1

Merged

ianhattendorf merged commit 0ac7af7 into nodegit:libgit-next Oct 28, 2022

Conversation

gitkraken-jacobw commented Jun 29, 2022

Uh oh!

AlexaXs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexaXs Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AlexaXs Jul 13, 2022 •

edited

Loading